Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add strip optimizations to linker flags #22

Merged
merged 3 commits into from
Apr 3, 2024
Merged

Conversation

nicholasjng
Copy link
Owner

Previously, mangled symbols would show up in the exports list on MacOS (as seen in nm -U commands on the Python bindings shared objects).

The fix here is to supply -Wl,-x and -Wl,-S to the macOS linker, which ends up producing the same output as CMake does (verified on my machine).

Similarly, after checking the Linux ninja build manifest, Linux needs a -Wl,-s.

Previously, mangled symbols would show up in the exports list on MacOS
(as seen in `nm -U` commands on the Python bindings shared objects).

The fix here is to supply `-Wl,-x` and `-Wl,-S` to the macOS linker,
which ends up producing the same output as CMake does (verified on my
machine).

Similarly, after checking the Linux ninja build manifest, Linux needs a
`-Wl,-s`.
@wjakob
Copy link

wjakob commented Apr 3, 2024

A quick comment: these flags impede debugging. Is there a way to apply them conditionally in release builds? This is what the nanobind cmake build system does.

@nicholasjng
Copy link
Owner Author

nicholasjng commented Apr 3, 2024

Thanks for the clarification. Perhaps this will work?

config_setting(
    name = "release_build",
    flag_values = {"compilation_mode": "opt"},
)

@nicholasjng
Copy link
Owner Author

@wjakob looks like this works, are there any more release-mode-only optimizations you can think of?

(I will redesign the skylib setting groups in a subsequent PR.)

@wjakob
Copy link

wjakob commented Apr 3, 2024

Nanobind tweaks the following depending on whether or not the user performs a release or a debug build.

  • stripping of linked binaries performed in release mode.
  • the -DNB_COMPACT_ASSERTIONS compile definition is set in release mode. (This removes a large set of error message strings that are mainly useful for development and debugging)

It's probably a good idea for you to replicate these two behaviors as well.

The following are more opinionated and with opt-outs:

  • size optimization (-Os) is enabled in release mode except unless the user specifies the NOMINSIZE flag.
  • the -fno-stack-protector compile is passed in release mode unless the user specifies the PROTECT_STACK flag.

The latter is only tangentially related to size optimizations, and has
to do with debug info, so we condition on release mode.
@nicholasjng
Copy link
Owner Author

Thanks, then I'm all set. I'm currently not supporting the PROTECT_STACK flag, but I'll note it down for the future.

Actually, the debug stripping had the neat side effect of the abi3audit passing again (though perhaps for the wrong reasons).

@nicholasjng nicholasjng merged commit 751a86c into master Apr 3, 2024
16 checks passed
@nicholasjng nicholasjng deleted the link-stripopts branch April 3, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants